-
Notifications
You must be signed in to change notification settings - Fork 766
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix deadlocks due to invalid handling of SSL sockets and select #504
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I don't think this is a common use case, so this is probably been overlooked for some time.
This looks mostly correct, except for a few notes.
# Otherwise, there could be still data available for reading, but select | ||
# wouldn't report the socket as readable again, since all data has already | ||
# been read by the SSL socket from the OS. | ||
# see also: https://docs.python.org/3/library/ssl.html#notes-on-non-blocking-sockets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it guarantees to give you everything in a single recv()
. For one thing, there might be multiple frames buffered. So you need to do what that link specifies and continue calling recv()
until it is drained. You have SSLSocket.pending()
to help you out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does. That's the good thing. Let me quote the documentation on OpenSSL's SSL_read()
:
The read functions work based on the SSL/TLS records. The data are received in records (with a maximum record size of 16kB). Only when a record has been completely received, can it be processed (decryption and check of integrity). Therefore, data that was not retrieved at the last read call can still be buffered inside the SSL layer and will be retrieved on the next read call. If num is higher than the number of bytes buffered then the read functions will return with the bytes buffered. If no more bytes are in the buffer, the read functions will trigger the processing of the next record. Only when the record has been received and processed completely will the read functions return reporting success. At most the contents of one record will be returned.
And unfortunately, SSLSocket.pending()
doesn't seem to work at all on some platforms. For me, it always returns zero regardless if there's something pending or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I read that text it supports my initial assertion. If there are multiple records buffered then it states that only the first record will be returned. So you need to call it again.
SSL sockets can't be used with
select()
in the same way raw sockets would, since the OS socket readable/writable state is not always identical to the one of the SSL stack. See also:https://docs.python.org/3/library/ssl.html#notes-on-non-blocking-sockets
For instance, websockify is currently unable to proxy a connection to a SSL target - unless the server-side sends data first.
In this case, the OS socket is reported as readable - due to SSL protocol data like sessions tickets being exchanged - while the SSL socket is not readable since application data hasn't been received from the server-side yet. As SSL sockets are read from (and written to) in a blocking fashion, the current implementation then simply stalls. For an example, see here:
Screen.Recording.2022-01-08.at.19.14.27.3.mov
The following patch tries to leave most of the existing socket handling logic intact while addressing the special handling required for SSL sockets. In particular, SSL sockets are now put in non-blocking mode, so that false-positive readable reports from
select()
don't lead to a deadlock but an exception. (Unfortunately, theSSLSocket.pending()
API doesn't seem to work reliably to archive the same with blocking SSL sockets.. :-/)In addition, the patch introduces an assertion to ensure the internal buffer size is equal or above 16kb so that all available data can be directly read from a SSL socket in a single call. Otherwise, there would be additional (annoying) handling required to make sure no data is accidentally left behind when a SSL socket would be only partially read. In this case,
select()
wouldn't report this socket as readable again - unless new data is received - since the rest of the data has already been read and buffered by the SSL socket layer.